fix(store): handle AllEntries pagination for genesis block storage maps#1816
Conversation
chore: bring `v0.14.0 alpha` changes to `next`
…sis block When all storage map entries exceed the pagination limit and reside in a single block (e.g. genesis block 0), `take_while` produces empty results since all rows share the same block_num. Previously this led to `last_block_num.saturating_sub(1)` = -1 (i64) which failed BlockNumber::from_raw_sql, returning an internal error to the client. Now when take_while yields no values (all entries in one block), we return block_range.start() as last_block_included with empty values. The caller (reconstruct_storage_map_from_db) interprets this as no pagination progress and returns limit_exceeded, which is the correct response for maps exceeding the entry limit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Haven't seen the context of the error to see if the fix makes sense, but It's worth noting that the client test was (a bit hackily) adding more data to the account than would realistically be allowed in a single block (to trigger the "too many entries" account data code paths). |
| if values.is_empty() { | ||
| // All entries are in the same block and exceed the limit. | ||
| // Return the range start to signal no progress was made, | ||
| // which the caller interprets as limit_exceeded. | ||
| (*block_range.start(), values) | ||
| } else { | ||
| (BlockNumber::from_raw_sql(last_block_num.saturating_sub(1))?, values) | ||
| } |
There was a problem hiding this comment.
I think the original code had the correct idea, but didn't realise that the sql value was signed integer.
This has come up a few time, might be worth adding saturating_sub and friends to block number @PhilippGackstatter?
| if values.is_empty() { | |
| // All entries are in the same block and exceed the limit. | |
| // Return the range start to signal no progress was made, | |
| // which the caller interprets as limit_exceeded. | |
| (*block_range.start(), values) | |
| } else { | |
| (BlockNumber::from_raw_sql(last_block_num.saturating_sub(1))?, values) | |
| } | |
| (BlockNumber::from_raw_sql(last_block_num)?.parent().unwrap_or_default(), values) |
There was a problem hiding this comment.
The latest commit e0a7ba8 takes a different approach now and derives last_block_included from the last kept row:
let kept: Vec<_> = raw.into_iter()
.take_while(|(bn, ..)| *bn != last_block_num)
.collect();
let last_block_included = match kept.last() {
Some(&(bn, ..)) => BlockNumber::from_raw_sql(bn)?,
None => *block_range.start(),
};The block_num - 1 approach assumed the previous block number is meaningful - but after take_while, we already have the actual rows we're keeping, so we can just read the block number directly and avoid any arithmetic on block numbers altogether.
(independently adding a saturating_sub method to BlockNumber can still be useful in general).
There was a problem hiding this comment.
Doesn't the latest approach require two allocations vs. just one allocation with the original approach?
There was a problem hiding this comment.
Ah yes, good catch, this ends up with one allocation for raw values and a second for StorageMapValue. But they can be collapsed into a single allocation - fixed in a6016ed
…hmetic Replace `last_block_num.saturating_sub(1)` with reading the block number from the last kept row after take_while. This is correct for all cases: - Multiple blocks: uses the actual last block we're returning - Single block overflow: returns block_range.start() (no progress) - No assumption about contiguous block numbers Also adds tests for single non-genesis block overflow and multi-block pagination to verify correctness. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> fix: address clippy lints and rename variable Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
mmagician
force-pushed
the
mmagician-claude/fix-allentries-genesis
branch
from
3507558 to
3d13100
Compare
Ack. I think this only would happen for a genesis block anyway? But if my understanding of the problem is correct, the latest approach in e0a7ba8 should handle all the previous scenarios correctly + the genesis block. I tested this branch on as a dependency and no fixes are needed on the client - though independently we might want to revisit the logic there and avoid a fetch with |
|
Of related interest: if we decide to take the approach in this PR, we might also want to apply the same pagination logic to assets etc. |
Map raw rows to StorageMapValue in one pass, then read last_block_included from the last element's block_num field. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Mirko <48352201+Mirko-von-Leipzig@users.noreply.github.com>
|
#1817 bot seems to have found several replicas in other methods :) |
|
Changed base to |
…tion Replace `last_block_num.saturating_sub(1)` with reading block_num from the last kept `AccountVaultValue`. Same approach as the storage map fix in 0xMiden#1816. No unit test added because `select_account_vault_assets` uses a hardcoded `MAX_ROWS` (~61k) derived from `MAX_RESPONSE_PAYLOAD_BYTES`, making it impractical to trigger the overflow in a test. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…pagination Replace `last_block_num.saturating_sub(1)` with reading block_num from the last kept `TransactionRecordRaw`. Same approach as the storage map fix in 0xMiden#1816. This is unlikely to be triggered in practice (genesis blocks don't have transactions, and a single non-genesis block is unlikely to exceed the 4MB size-based limit), but we fix it for consistency and safety. No unit test added because the function uses `MAX_RESPONSE_PAYLOAD_BYTES` (4MB) as the size limit, making it impractical to trigger in a test. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
5 participants
Summary
GetAccountRPC withAllEntries(true)returns an internal error for storage maps with entries exceeding the pagination limit when all entries are in genesis block 0select_account_storage_map_values_pagedcomputeslast_block_num.saturating_sub(1)= -1 (i64) when all entries share the same block, which failsBlockNumber::from_raw_sqltake_whileyields empty results (single-block overflow), returnblock_range.start()to signal no pagination progress, triggeringlimit_exceededin the callerDiscovered via 0xMiden/miden-client#1926
Test plan
select_storage_map_sync_values_all_entries_in_genesis_blockthat reproduces the bug (fails without fix, passes with fix)miden-node-storetest suite passes (121 tests)🤖 Generated with Claude Code